Skip to content

Make sure mapping is not stateful #4475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 17, 2021
Merged

Make sure mapping is not stateful #4475

merged 2 commits into from
May 17, 2021

Conversation

thomasp85
Copy link
Member

This PR fixes the "statefulness" of the mapping field introduced in #4265 by aligning it with how we now treat geom_params and stat_params. In fact the state has existed since the introduction of LayerSf where the mapping would be modified to ensure the geometry was catched.

This clean-up leaves a couple of dangling issues, mainly the fact that default_mapping no longer needs to be passed around. It is kept as-is so we don't change the signature of the guide calculation and break extension packages. We can remove it entirely when we rewrite the guide functionality to ggproto

@thomasp85 thomasp85 added this to the ggplot2 3.3.4 milestone May 11, 2021
@clauswilke
Copy link
Member

Sorry if I'm dense. Could you explain in a few more sentences what exactly is being done?

Also, if default_mapping is kept just to maintain the signature, can you point that out in a comment and/or add a check that it's actually a missing argument? Or alternatively, replace with ...?

@thomasp85
Copy link
Member Author

plot-level mappings and layer-level mappings needs to be fused together - earlier this was done all over the place and let to bugs which resulted in us moving the fusing into setup_layer(). This altered the state of the layer since it potentially modified the content of the mapping field. This was already being done in LayerSf where a geometry mapping was added if missing.

With this PR mapping is never altered, but the fused mapping is stored in a computed_mapping field similar to what is being done for computed_geom_params and computed_stat_params. This means that apart from computed_* fields layers remain stateless and are easier to reason about.

I already added a note at the guide_geom generic that default_mapping is unused and should be removed come rewrite

clauswilke
clauswilke approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looks good to me.

I notice that there doesn't seem to be a unit test for the inherit.aes argument, so maybe add a very simple one.

yutannihilation
yutannihilation approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It seems I was kind of aware about the need to treat the statefulness at the time of #4265 (comment), but couldn't come up with any solutions. Thanks for catching!

@thomasp85 thomasp85 merged commit 1cede18 into v3.3.4-rc May 17, 2021
@cpsievert cpsievert mentioned this pull request Jun 17, 2021
Merged
@thomasp85 thomasp85 mentioned this pull request Apr 14, 2023
Merged
teunbrand added a commit to teunbrand/ggplot2 that referenced this pull request Apr 16, 2023
@teunbrand
Purge default_mapping arguments, see tidyverse#4475
365b3f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yutannihilation yutannihilation yutannihilation approved these changes

@clauswilke clauswilke clauswilke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
ggplot2 3.3.4
Development

Successfully merging this pull request may close these issues.
3 participants
@thomasp85 @clauswilke @yutannihilation